stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/StreamMediaPlayerContent.kt (1)

86-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider defaulting useVideoCache to false.

The default is true, so any caller that omits the argument routes playback through the video cache — including non-video (e.g., audio) content, which contradicts the "video only" intent documented for this parameter. Current in-cohort callers pass it explicitly, so this is only a guard against future misuse; a conservative false default would make the opt-in semantics safer.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/StreamMediaPlayerContent.kt`
at line 86, The default value for useVideoCache in StreamMediaPlayerContent is
too permissive; update it to false so cache usage remains an explicit opt-in and
doesn’t affect callers that omit the argument. Adjust the default on the
StreamMediaPlayerContent parameter, keeping the existing explicit call sites
unchanged, and ensure the parameter’s intent remains “video only” in its usage.

stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/StreamCacheConfig.kt (1)

19-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add thread/state notes to the new public KDoc.

These are new public client APIs, but the KDoc still omits the thread-safety/state expectations required for this module's public surface. A short note that the configs are immutable value objects and that caching stays disabled unless video is non-null would close that gap. As per coding guidelines "Document public APIs with KDoc, including thread expectations and state notes".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/StreamCacheConfig.kt`
around lines 19 - 45, The new public KDoc on StreamCacheConfig and
VideoCacheConfig needs thread/state expectations added. Update the docs for
StreamCacheConfig and its video property to note that these are immutable value
objects and that video caching remains disabled unless video is non-null. Also
add a brief state note to VideoCacheConfig in the same style so the public API
documentation matches the module guidelines.

Source: Coding guidelines

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoMediaCache.kt (2)

90-105: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Registry entry removed before teardown succeeds (still unresolved from prior review).

instances.remove(dirPath) at line 93 runs before cache.release()/databaseProvider.close(). If either throws (caught and only logged), a subsequent create() for the same directory will construct a new SimpleCache while the old one may still hold Media3's directory lock, causing the new construction to fail.

🔒 Proposed fix

     internal fun release() {
         synchronized(instances) {
-            instances.remove(dirPath)
+            var released = true
             try {
                 cache.release()
             } catch (`@Suppress`("TooGenericExceptionCaught") e: Exception) {
                 logger.e(e) { "[release] failed to release SimpleCache" }
+                released = false
             }
             try {
                 databaseProvider.close()
             } catch (`@Suppress`("TooGenericExceptionCaught") e: Exception) {
                 logger.e(e) { "[release] failed to close StandaloneDatabaseProvider" }
+                released = false
             }
+            if (released) {
+                instances.remove(dirPath)
+            }
         }
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoMediaCache.kt`
around lines 90 - 105, The release flow in VideoMediaCache.release removes the
registry entry before teardown finishes, so a failed cache.release() or
databaseProvider.close() can leave the old Media3 lock active while create()
builds a new instance. Move instances.remove(dirPath) to after both teardown
calls succeed, or otherwise only unregister once release and close have
completed without error, using VideoMediaCache.release, create(), and the
instances map as the key symbols to update.

---

123-142: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Singleton key still uses raw absolutePath (still unresolved from prior review).

cacheDir.absolutePath doesn't normalize aliases like ./video-cache vs video-cache or symlinked equivalents, so the "one SimpleCache per directory" guard can still be bypassed for the same physical directory.

Proposed fix

         public fun create(appContext: Context, cacheDir: File, config: VideoCacheConfig): VideoMediaCache =
             synchronized(instances) {
                 cacheDir.mkdirs()
-                val key = cacheDir.absolutePath
+                val normalizedDir = cacheDir.canonicalFile
+                val key = normalizedDir.path
                 instances[key]?.let { existing ->
                     ...
                 }
                 val dbProvider = StandaloneDatabaseProvider(appContext)
                 val simpleCache = SimpleCache(
-                    cacheDir,
+                    normalizedDir,
                     LeastRecentlyUsedCacheEvictor(config.maxSizeBytes),
                     dbProvider,
                 )
                 VideoMediaCache(simpleCache, dbProvider, key).also { instances[key] = it }
             }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/cache/internal/VideoMediaCache.kt`
around lines 123 - 142, The singleton lookup in VideoMediaCache.create still
uses cacheDir.absolutePath, so equivalent directories can bypass the reuse
guard. Update the key generation to use a normalized canonical/real path for the
cache directory before checking instances and storing the cache, and keep the
reuse logic in create synchronized on that normalized key.

stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaPreviewActivity.kt (1)

187-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Reconsider adding @JvmOverloads here.

Based on learnings, this module intentionally avoids @JvmOverloads for public API compatibility and relies on Kotlin default arguments instead. Please confirm there's a specific Java-interop need driving this addition; otherwise consider dropping @JvmOverloads to stay consistent with the established convention.

♻️ Suggested diff

-        `@JvmOverloads`
         public fun getIntent(
             context: Context,
             url: String,
             title: String? = null,
         ): Intent {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaPreviewActivity.kt`
around lines 187 - 192, The public MediaPreviewActivity.getIntent API should not
introduce `@JvmOverloads` unless there is a concrete Java-interop requirement;
this module prefers Kotlin default arguments for public compatibility. Remove
`@JvmOverloads` from getIntent unless you can justify the need, and keep the
signature aligned with the established convention used in this compose module.

Source: Learnings

stream-chat-android-compose/api/stream-chat-android-compose.api (1)

979-981: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

New @JvmOverloads-generated overload conflicts with an established module convention.

This entry is generated correctly, but it's a direct byproduct of adding @JvmOverloads to MediaPreviewActivity.Companion.getIntent (see that file). Based on learnings, in this module Kotlin default arguments should be relied on instead of introducing @JvmOverloads-generated overloads for public API compatibility.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@stream-chat-android-compose/api/stream-chat-android-compose.api` around lines
979 - 981, Remove the public overloads introduced by `@JvmOverloads` on
MediaPreviewActivity.Companion.getIntent and rely on Kotlin default arguments
instead. Update the Companion.getIntent API so it exposes only the intended
single Kotlin entry point with defaulted parameters, and regenerate the .api
metadata to drop the extra getIntent overload and getIntent$default symbol.

Source: Learnings